I also added an arm action for allowing relative movements (it's quite cumbersome to always have to specify absolute coordinates).

Relative movements have already been supported. You just have to change the coordinate space. See: https://ai2thor.allenai.org/manipulathor/documentation/#MoveArm-coordinatespace.

Edit, weird. On some devices I'm getting an issue with the videos where they appear in weird positions covering the text. Looking into what's happening here.

This pull request introduces 1 alert and fixes 1 when merging aea035b into 35d7922 - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

fixed alerts:

• 1 for Useless assignment to local variable

The de-sync between the wrist position and its out-of-range pos_rot_mainpulator was a working-as-intended decision, but if re-syncing them after the fact makes more sense, I'm all for it.

@mattdeitke From my understanding, changing the coordinate frame gets us part of the way there but has a couple of deficiencies:

1. The only way to really move things in a relative (or offset) fashion is to pick the coordinate frame of the wrist. This works fine if the wrist isn't rotated but if I do rotate the wrist then I can't do relative movements w.r.t. to the base (which is the most natural type of relative moment from the agent's perspective in my mind).
2. What if I want to do relative movements but in the global coordinate frame? I don't think this is supported at all.

The only way to really move things in a relative (or offset) fashion is to pick the coordinate frame of the wrist. This works fine if the wrist isn't rotated but if I do rotate the wrist then I can't do relative movements w.r.t. to the base (which is the most natural type of relative moment from the agent's perspective in my mind).

@Lucaweihs I think you can just use the armBase coordinate space to make sure things are always relative to wherever the arm's base is. Perhaps this is enough and we don't need a full new action in order to accomplish this? Perhaps I'm misunderstanding what you mean by the "base" though, base of the wrist, base of the arm, the agent itself?

What if I want to do relative movements but in the global coordinate frame? I don't think this is supported at all.
Using the world coordinate space is relative to a global coordinate frame. The world origin is always constant, so it will always be relative to that origin. Again, perhaps I'm misunderstanding what you are trying to do here.

Also @elimvb do you remember why we decided it was working as intended? I recall an early bug where the pos rot manipulator was desynced causing the final position of the wrist to not end up where it was expected, like it was offset by a couple of delta time increments. Do you recall if this is related to that?

@Lucaweihs I think you can just use the armBase coordinate space to make sure things are always relative to wherever the arm's base is. Perhaps this is enough and we don't need a full new action in order to accomplish this? Perhaps I'm misunderstanding what you mean by the "base" though, base of the wrist, base of the arm, the agent itself?

I don't believe that works, here's an example:

• Suppose that the arm is at position (1, 1, 1) in the armBase coordinate space.
• I now want to move the arm to the right by 0.1m in the armBase coordinate space.

How can I do this? If I use the MoveArm function with the armBase coordinate space then I have to explicitly tell it to move the arm to position (1.1, 1, 1) rather than telling it to move by (0.1, 0, 0). This is super unpleasant because I might not know the exact position of the arm in the armBase coordinate space (it's not being returned in the metadata to my understanding) and, even if the position of the arm was being returned in every conceivable coordinate space, it would mean that every manipulathor user would have to do these computations themselves.

Ah I see what you mean now. This sort of "move some amount relative to my current coordinate" does currently only work when in the wrist relative space, since it is the wrist that is moving so its coordinate space effectively moves with it. This sort of "move right of where I am currently by 0.1m" is currently required to be calculated on the user end when using other coordinate spaces.

I'm not sure if this should be a whole new action as you have added or if it should just be another parameter included in the default arm movement actions. I could see something like specifying coordinateSpace=armBase and then moveRelativeToCurrentPos=True or something like that. I think we can probably come up with a better name for this, but I'm currently leaning towards integrating this into the current MoveArm action since its already sort of supported when using wrist coordinate space.

This pull request introduces 1 alert and fixes 1 when merging 6966a37 into 35d7922 - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

fixed alerts:

• 1 for Useless assignment to local variable

Also @elimvb do you remember why we decided it was working as intended? I recall an early bug where the pos rot manipulator was desynced causing the final position of the wrist to not end up where it was expected, like it was offset by a couple of delta time increments. Do you recall if this is related to that?

@winthos I don't recall an error like the one you're describing. The working-as-intended decision was because I wanted the pos_rot_manipulator's transform to work independently of the arm's range. So if the manipulator was put out of the agent's reach, the arm would hyperextend as much as it could to reach it. Thus, the wrist transform wouldn't be synced with manipulator in this case, but instead get as close as it could without breaking the arm. However, Luca made the valid point that this forces us to keep track of two pieces of end-joint metadata, which gets really confusing when you're doing follow-up transforms. So he added an extra step where, once the wrist gets as close as it can to the pos_rot_manipulator, the manipulator's work is complete for that step, and snaps over to the wrist's location so that they are both synced for the next transform. That seems like an intuitive compromise of logic to me, given our variables.

@elimvb Gotchya. I agree, this change is a good compromise given the behavior that we are expecting.

Overall I think this looks good to go, but I do want @ehsanik to check to make sure none of the changes break anything on her end.

This pull request introduces 1 alert and fixes 1 when merging 511fedf into edc4c37 - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

fixed alerts:

• 1 for Useless assignment to local variable